New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xds: fail to create xDS channel if no server with supported channel creds found #7400
xds: fail to create xDS channel if no server with supported channel creds found #7400
Conversation
c1f54ce
to
612b791
Compare
@@ -129,8 +128,10 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | |||
xdsClientPool = attributes.get(XdsAttributes.XDS_CLIENT_POOL); | |||
if (xdsClientPool == null) { | |||
final BootstrapInfo bootstrapInfo; | |||
final XdsChannel channel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This EDS-only codepath will be deleted in #7391.
@@ -687,8 +623,4 @@ boolean isUseProtocolV3() { | |||
return useProtocolV3; | |||
} | |||
} | |||
|
|||
interface XdsClientPoolFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More cleanup to be done to put RefCountedXdsClientObjectPool into its own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be easier to review if the first commit Move XdsChannelFactory to its own file and improve tests.
is split into two commits with one pure move and the other pure modification; and moving interface XdsClientPoolFactory
is not scattered in the first and third commit.
@Test | ||
public void defaultUseV2ProtocolL() throws XdsInitializationException { | ||
XdsChannel channel = createChannel(server1); | ||
assertThat(channel.isUseProtocolV3()).isFalse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also check
server1 with experimentalV3SupportEnvVar = true
and
server2 with experimentalV3SupportEnvVar = false
still do not support v3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Doesn't need to be that throughout, right? It makes tests noisy to test trivial things deeply.
server1 with experimentalV3SupportEnvVar = true and server2 with experimentalV3SupportEnvVar = false
This really just tests the functionality of the environment variable, which is a temporary gate. IMO, we shouldn't bother testing it. Unit tests can also introduces bugs, so if something is trivial in implementation, we won't bother write tests for it.
I am sorry, commits are a bit screwed up.. 😢 |
*/ | ||
@Override | ||
XdsChannel createChannel(List<ServerInfo> servers) throws XdsInitializationException { | ||
checkArgument(!servers.isEmpty(), "No management server provided."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well throw XdsInitializationException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Fixed.
…he xDS channel is empty.
…reds found (grpc#7400) Create the xDS channel outside the XdsClient. Throw an XdsInitializationException if the provided server list (parsed from the bootstrap file) can not be used to create such a channel. The exception is caught by the xDS resolver and propagated to the Channel gracefully as a name resolution error.
Follow up for #7396.
The main change is to create the xDS channel outside XdsClient and creating the xDS channel can fail.